Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

動けるようにする #1288

Open
wants to merge 39 commits into
base: fix/openapi
Choose a base branch
from
Open

動けるようにする #1288

wants to merge 39 commits into from

Conversation

Eraxyso
Copy link

@Eraxyso Eraxyso commented Dec 19, 2024

Closes #1261

controller testに関する部分はあと #1271 でやります

変更箇所リスト:

  • wire(これに伴いcontroller/middleware -> handler/middleware)
  • OapiRequestValidatorを削除(Authenticationに関する部分がうまくいけないので)
  • openapiのlocal server urlの更新
  • CI workflowの更新(バージョンのアップデート、setup-goのcacheとactoins/cacheの機能が重複するのでsetup-goのcacheをオフにした...)
  • model test & lintが通れるようにいろいろな修正(一部使わない変数、関数とかがコメントアウトされた...)
  • アンケート対象者&管理者のグループ管理の実装

@Eraxyso Eraxyso marked this pull request as ready for review January 14, 2025 06:17
@Eraxyso Eraxyso requested a review from kaitoyama January 14, 2025 06:17
@kaitoyama
Copy link
Contributor

@Eraxyso ありがとうございます!
結構色々修正してもらうことになってしまったようなので、どういう意図の変更をしたのかのリストを書いて欲しいです

  • 〇〇がうまく行くように◇◇を△△に

みたいな感じでお願いしたいです

@Eraxyso
Copy link
Author

Eraxyso commented Jan 16, 2025

↑prの説明に追記しました

Copy link
Contributor

@kaitoyama kaitoyama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

基本的に良さそうです、少しだけ確認したいことがあったのでコメントしました。

description: 回答したもの(answered)か未回答のもの(unanswered)かを選別
schema:
$ref: "#/components/schemas/AnsweredType"
# answeredInQuery:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この辺は使ってなかったからコメントアウトした感じ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

その通りです。Lintが警告を出しましたので

args: args{
userID: questionnairesTestUserID,
sort: "",
search: "",
pageNum: 100000,
pageNum: 1,
onlyTargetingMe: true,
onlyAdministratedByMe: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

すごい直感に反する命名なので、後でschemaから変えた方が良いかも。今はこれでOK

自分が管理者になっていないもののみ取得 (true), 管理者になっているものも含めてすべて取得 (false)。デフォルトはfalse。

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

今は多分管理者になっていないものを取得するのではなく、管理者であるもののみを取得する実装です。以前はnontargetがtrueの場合ターゲットではないものを取得していましたが、何かの新機能を移植するときロジックが逆になったようです。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants